Skip to content

test: 100% line coverage + CI coverage gate#238

Open
AkashBrowserStack wants to merge 2 commits into
masterfrom
test/coverage-100-and-ci-gate
Open

test: 100% line coverage + CI coverage gate#238
AkashBrowserStack wants to merge 2 commits into
masterfrom
test/coverage-100-and-ci-gate

Conversation

@AkashBrowserStack

@AkashBrowserStack AkashBrowserStack commented Jun 12, 2026 •
edited
Loading

Copy link
Copy Markdown

What & why

This SDK had no coverage enforcement in CI and sat at 86% locally. This PR raises it to 100% line coverage with meaningful tests and wires a hard CI gate so coverage can't silently regress.

Every line is executed by a real unit test — no # pragma skips, no coverage-gaming. Even the defensive optional-import fallbacks are exercised via reload-based tests rather than excluded.

Tests added

Area Coverage added
snapshot.py (new test_snapshot_units.py) log() CLI-ship failure; _wait_for_ready script-timeout adjust/restore failures; CORS-iframe origin-error skip; get_responsive_widths non-list + request failure; CDP resize → set_window_size fallback; _responsive_sleep invalid/unset; capture_responsive_dom invalid minHeight
percy/__init__.py (new test_init.py) percy_screenshot dispatch (unsupported driver, RemoteConnection → automate, AppiumConnection delegation when appium installed + helpful error when not, unknown → None); and reload-based tests that execute the robot-library and percy_snapshot optional-import fallbacks
robot_library.py parse-helper edge cases, full _parse_padding matrix, _get_driver missing-SeleniumLibrary error, Percy Screenshot keyword (+ region elements), reload-based test of the no-robotframework stub
driver_metadata.py newer-Selenium client_config.remote_server_addr fallback

Instrumentation / gate

  • .coveragerc — source = percy, fail_under = 100, show_missing.
  • coverage + pytest-cov added to development.txt.
  • New make coverage runs every module (snapshot under percy exec --testing), combines parallel data, and enforces the threshold.
  • test.yml runs make coverage across the 3.8–3.10 matrix; new modules also added to make test.

Verification

make coverage -> TOTAL 514 stmts 0 miss 100%  (no pragma exclusions)  exit 0
pylint: 10.00/10

🤖 Generated with Claude Code

@AkashBrowserStack @claude
test: reach 100% line coverage and enforce a CI coverage gate
8131342 
Raise unit coverage from 86% to 100% line coverage with meaningful tests
and wire a hard gate into CI so it cannot regress.

Tests added:
- test_snapshot_units.py (new): targeted unit tests for snapshot.py error
  and fallback paths the e2e suite doesn't reach — log() shipping failure,
  _wait_for_ready script-timeout adjust/restore failures, CORS-iframe
  origin error skip, get_responsive_widths non-list + request failure,
  CDP resize fallback to set_window_size, _responsive_sleep invalid/unset
  time, and capture_responsive_dom invalid minHeight handling.
- test_init.py (new): percy_screenshot dispatch — unsupported driver,
  RemoteConnection -> automate, AppiumConnection delegation when the
  appium package is present, helpful error when it is not, and unknown
  connection returning None.
- robot_library: parse-helper edge cases (_parse_widths/_parse_csv/
  _parse_json unsupported types, full _parse_padding matrix),
  _get_driver SeleniumLibrary-missing error, percy_screenshot keyword
  (basic + ignore/consider region elements), and a reload-based test of
  the no-robotframework stub (graceful ImportError).
- driver_metadata: command_executor._url -> client_config.remote_server_addr
  fallback for newer Selenium clients.

Instrumentation:
- Add .coveragerc (source=percy, fail_under=100, show_missing).
- Add coverage to development.txt; new Makefile `coverage` target runs
  every test module (snapshot under `percy exec --testing`), combines the
  parallel data, and enforces the threshold.
- test.yml runs `make coverage` across the Python matrix.
- Add the two new modules to `make test` as well.
- __init__: mark two genuinely-unreachable optional-import fallbacks with
  `# pragma: no cover` (robot_library never raises ImportError; percy.snapshot
  is the core module and always ships).
- gitignore coverage/npm artifacts.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@AkashBrowserStack AkashBrowserStack requested a review from a team as a code owner June 12, 2026 12:14
@AkashBrowserStack @claude
test: cover optional-import fallbacks with real tests, drop pragmas
083ef0e 
Replace the two no-cover pragmas in percy/__init__ with reload-based
unit tests that actually execute the fallback lines:
- block percy.robot_library so the robot import fails and the
  except: pass branch runs (package still loads)
- inject a percy.snapshot stand-in lacking percy_snapshot so the
  import fails and the ModuleNotFoundError fallback is defined and
  raised.
Coverage stays at 100% line with no skipped lines.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@AkashBrowserStack

AkashBrowserStack commented Jun 15, 2026

Copy link
Copy Markdown
Author

Claude Code PR Review

PR: #238 • Head: 083ef0e • Reviewers: stack:code-reviewer

Summary

Adds focused unit tests for the package entrypoint, snapshot.py helper/fallback paths, robot_library.py parsers and keywords, and a driver_metadata Selenium-version fallback; wires a coverage Make target plus .coveragerc with a 100% fail_under line-coverage gate and switches CI from make test to make coverage.

Review Table

Priority Category Check Status Notes
High Security No hardcoded secrets or credentials Pass Tests use only synthetic fixture values (fallback-hub, https://page); no real secrets.
High Security Authentication/authorization checks present N/A No auth surface; PR is test + CI tooling only.
High Security Input validation and sanitization N/A No new user-input boundaries introduced.
High Security No IDOR — resource ownership validated N/A No resource access in scope.
High Security No SQL injection (parameterized queries) N/A No database/SQL in scope.
High Correctness Logic is correct, handles edge cases Pass Tests faithfully target real branches (Selenium _url→remote_server_addr fallback, resize fallback, min-height parse failure, responsive widths upgrade hint, swallowed log/timeout failures). All 53 new/modified tests pass locally.
High Correctness Error handling is explicit, no swallowed exceptions Pass Test code raises/asserts explicitly; the swallow paths it exercises are pre-existing production behavior (e.g. best-effort log POST), not introduced here.
High Correctness No race conditions or concurrency issues Pass importlib.reload global-state mutations in test_init/test_robot_library are guarded by try/finally: importlib.reload(...) restore; combined run is order-independent and green.
Medium Testing New code has corresponding tests Pass This PR is the tests; the coverage/.coveragerc tooling is exercised by the CI make coverage switch.
Medium Testing Error paths and edge cases tested Pass Strong negative-path focus: unsupported types, import failures, non-numeric min-height/sleep, network failures, un-parseable iframe origin.
Medium Testing Existing tests still pass (no regressions) Pass Modified test_driver_metadata.py and test_robot_library.py add cases without altering existing ones; suites pass.
Medium Performance No N+1 queries or unbounded data fetching N/A Test code; no production data paths added.
Medium Performance Long-running tasks use background jobs N/A Not applicable to tests.
Medium Quality Follows existing codebase patterns Pass unittest + unittest.mock style consistent with existing test_cache.py/test_driver_metadata.py; pylint disables scoped narrowly.
Medium Quality Changes are focused (single concern) Pass Single concern: raise coverage + enforce it. .gitignore/development.txt additions support that concern.
Low Quality Meaningful names, no dead code Pass Descriptive test names; explanatory comments justify the reload/dispatch tricks. No dead code.
Low Quality Comments explain why, not what Pass Comments explain intent (why a class is named WebDriver, why imports are forced to fail).
Low Quality No unnecessary dependencies added Pass Only coverage==7.* added to development.txt (dev-only, standard tool).

Findings

No Critical or High findings. Minor (Low) observations, non-blocking:

  • File: .coveragerc:6 and .github/workflows/test.yml:64

  • Severity: Low

  • Reviewer: stack:code-reviewer

  • Issue: fail_under = 100 (line coverage) is strict. It is met today, but any future production line not reachable by a fast unit test will fail CI even if e2e-covered, since only line coverage is gated (no branch = True). This is the intended design of the PR, noted for awareness, not a defect.

  • Suggestion: Acceptable as-is. If branch completeness is later desired, add branch = True under [run]; otherwise leave the 100% line gate.

  • File: .gitignore:35

  • Severity: Low

  • Reviewer: stack:code-reviewer

  • Issue: package-lock.json is newly ignored although a package-lock.json is already committed at repo root. Ignoring a tracked file has no effect on the tracked copy and can be mildly confusing.

  • Suggestion: Either git rm --cached package-lock.json if it should not be tracked, or drop that ignore line. Cosmetic only.

  • File: tests/test_snapshot_units.py:452-468

  • Severity: Low

  • Reviewer: stack:code-reviewer

  • Issue: TestCaptureResponsiveDomMinHeight writes output_file.json to the CWD (production capture_responsive_dom opens it relative to CWD); cleanup is in tearDown, but a failure between write and teardown could leave the artifact. It is now in .gitignore, so impact is contained.

  • Suggestion: Fine as-is given the .gitignore entry; optionally chdir into a tmp dir within the test for full isolation.

Verdict: PASS — tests are faithful to the real code paths, all 53 pass locally, and the coverage gate tooling is internally consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rishigupta1599 rishigupta1599 rishigupta1599 approved these changes

@amandeepsingh333 amandeepsingh333 Awaiting requested review from amandeepsingh333 amandeepsingh333 is a code owner automatically assigned from percy/percy-product-reviewers

@Shivanshu-07 Shivanshu-07 Awaiting requested review from Shivanshu-07 Shivanshu-07 is a code owner automatically assigned from percy/percy-product-reviewers

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@AkashBrowserStack @rishigupta1599